-
Notifications
You must be signed in to change notification settings - Fork 2
Feat / Refactor perils and add tab navigation #659
Conversation
ce075d2
to
4950000
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
I realized after adding comments that you probably just copy-pasted from web-onboarding without any regards to improving the code, in which case you can just disregard my suggestions 😄
No that's great! It was just me who missed it so thanks for pointing it out 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! A lint warning that could be nice to get rid of, otherwise LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! This is almost nostalgic - I remember how hard/long I worked on this tab solution to make it accessible and get the damn line to animate between the tabs 😅
import { LAPTOP_BP_UP } from '../blockHelpers' | ||
import { UnderlineComponent, UNDERLINE_HEIGHT } from './Underline' | ||
|
||
const TabContainer = styled.button<{ selected?: boolean }>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we override the focus state globally? Don't remember, otherwise would be nice to add one here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean adding a focus state for the tabs? @robinandeer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah for these buttons -- but maybe it's covered in some other way?
We couldn't add a more recent version without editing the webpack settings and we had to add tslib
067f832
to
07c5228
Compare
Thanks haha but you did all the heavy lifting for this one :D A tricky thing indeed with the line animation! 🙏 @robinandeer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool stuff!
What?
Add tab navigation for Perils instead of current dropdown navigation.
I moved over the Tabs component from web-onboarding.
4.1.17
(issue described here: [BUG] 5.0 Failed to compile with create-react-app framer/motion#1307) and that's the same version we use inonboarding
. That did cause another issue as well wheretslib
complainedWARNING in ./node_modules/popmotion/dist/es/utils/mix-complex.js 22:15-28 "export '__spreadArray' was not found in 'tslib'
so we had to add that as a dependency as well. More on that issue here:[BUG] framer-motion breaks in tslib version 2.2.0 framer/motion#1123
Perils
component to simply accept a collection of perilsPerilsCollection
. I therefore renamed some of the components to make more sense.usePerils
to fetch multiple peril typesWhy?
Consistent UI with Perils in onboarding
Screenshots / recordings
Storybook
Screen.Recording.2022-05-03.at.15.44.06.mov
Storyblok
Screen.Recording.2022-05-04.at.10.27.41.mov